-
Notifications
You must be signed in to change notification settings - Fork 19
getattr() doesn't work on dicts, use .get() instead #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@aarontusko did you end up having a different work around or something that made these changes not needed? |
No. |
@aarontusko sorry we didn't get to it a bit quicker. We appreciate your contribution. I'm not super familiar with this library and it's usage, so I'm not sure if or to what extent I can test (i'll see if I can learn enough to do some useful testing), but I am definitely willing to help get the PR passing the CI checks if you're interested in re-opening. Feel free to re-open it if you'd like I've got it on my list to take a closer look into later on tonight. |
@aarontusko thank you for raising this issue and making the PR to resolve it! I apologize again that no one followed up on this PR sooner. I confirmed the root of this issue and proposed solution are correct with REPL:
It looks like only one of the device dictionaries contains the
The rest of them don't have a For the ATtiny13a it does have a I successfully tested the library with the changes from this PR using: https://github.com/adafruit/Adafruit_CircuitPython_AVRprog/blob/main/examples/avrprog_program_uno328.py and confirmed it is still behaving as expected. Since that chip doesn't have I don't think I have access to an It seems that Github is no longer showing the full output from the actions check so I can't see the specific reason that it failed but I am looking into that locally and on a new branch in my fork of the repo. Judging from the relatively small amount of actual code change I think it should be fairly straightforward to get this passing actions and then we can get it merged after that. @aarontusko if you still have the ability to restore your branch and are willing to do that please do. I will help get actions passing and get this merged. |
I made a new PR with the same changes from this one to try to get a look at why the actions may not have passed. The new PR is passing as-is. I think that it's likely the reason this one did not originally pass was unrelated to the changes actually from this PR. There have been some changes to PyLint checks across all libraries that started enforcing the use of I think it would be great if we can get this branch restored and merge the fix from here. @aarontusko it seems to me that you did a great job with everything in this PR so congrats on your first github contribution / pull request. |
@FoamyGuy Yay! Thank you for the time, help and encouragement. Your example demonstrates the issue and solution perfectly. The "clock_speed" key is optional in the Boards dicts, so if it is not there the code will just use a default value. In my case I had two different targets to program. Unfortunately I already deleted my fork, so I don't think I can restore the branch now. I guess I will have to make another fork? Thanks again |
@aarontusko If you are up for it please do make another fork and add these changes again. I was reading around a bit and it seems like we may not be able to get it to "hook up" back to this PR from a a new fork. But you can create a new PR once you do have a new fork and we'll link it back to this one with a comment so that folks will be able to see the history of it. |
@FoamyGuy Ok, I made a new fork and pull request. |
@aarontusko alright new one is merged and release has been made with it. The updated version will appear in the library bundle starting tomorrow and in the meantime can be obtained here on Github. You're very welcome, I'm happy to help out. Thank you again for contributing this fix and congrats once more on your first contribution and PR 🎉 If in the future you end up making other PRs to this or any of the other CircuitPython libraries and don't get a response within a few business days please feel free to @ mention me in a comment on the PR and I'll get the right set of eyes on to it. |
getattr() doesn't work on dicts
use .get() instead